fix(clerk-js): Correct race condition when fetching tokens #7304
Conversation
🦋 Changeset detectedLatest commit: 26f85df The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a changeset for a patch release; introduces a SafeLock API with timeouts and degraded fallback; removes cross-tab locking from SessionCookiePoller; adds per-token cross-tab locks in Session.ts to serialize token fetches; reorders cookie/devBrowser initialization in AuthCookieService; and adds tests for SafeLock and the poller. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/clerk-js/src/core/auth/safeLock.ts (1)
16-38: Fix inconsistent return type behavior.The
acquireLockAndRunfunction has inconsistent return behavior:
- Navigator.locks path (line 25-28): Returns
falseon error via.catch()- Fallback path (line 31-37): Returns
undefinedimplicitly when lock acquisition fails (after line 37)This creates ambiguity for callers: should they check for
false,undefined, or both to detect failure?Consider one of these approaches:
Option 1 (Recommended): Return a consistent sentinel value
const acquireLockAndRun = async (cb: () => Promise<unknown>) => { if ('locks' in navigator && isSecureContext) { const controller = new AbortController(); const lockTimeout = setTimeout(() => controller.abort(), 4999); return await navigator.locks .request(key, { signal: controller.signal }, async () => { clearTimeout(lockTimeout); return await cb(); }) .catch(() => { // browser-tabs-lock never seems to throw, so we are mirroring the behavior here return false; }); } if (await lock.acquireLock(key, 5000)) { try { return await cb(); } finally { await lock.releaseLock(key); } } + + // Return false when lock acquisition fails to match navigator.locks behavior + return false; };Option 2: Use a Result/Option type for clearer semantics
This would make success/failure explicit but requires more extensive changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/fix-token-refresh-race-condition.md(1 hunks)packages/clerk-js/src/core/auth/AuthCookieService.ts(6 hunks)packages/clerk-js/src/core/auth/SessionCookiePoller.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/auth/AuthCookieService.ts (4)
packages/clerk-js/src/core/auth/cookies/clientUat.ts (2)
ClientUatCookieHandler(12-15)createClientUatCookie(23-65)packages/clerk-js/src/core/auth/SessionCookiePoller.ts (2)
SessionCookiePoller(9-42)REFRESH_SESSION_TOKEN_LOCK_KEY(6-6)packages/clerk-js/src/core/auth/cookies/session.ts (2)
SessionCookieHandler(10-14)createSessionCookie(28-59)packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLockReturn(3-5)SafeLock(7-41)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLockReturn(3-5)SafeLock(7-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
6-6: Good: Exporting the lock key enables cross-component coordination.Exporting
REFRESH_SESSION_TOKEN_LOCK_KEYallowsAuthCookieServiceto create a shared lock using the same key, which is essential for the cross-tab synchronization mechanism.packages/clerk-js/src/core/auth/AuthCookieService.ts (4)
72-75: Excellent: Clear documentation of the locking mechanism.The comment clearly explains the purpose of the
tokenRefreshLockand how it coordinates cross-tab token refresh. This helps future maintainers understand the synchronization strategy.
137-137: Good: Shared lock injected into poller.Passing
this.tokenRefreshLockto theSessionCookiePollerconstructor ensures both the poller and the focus handler use the same cross-tab lock, which is essential for preventing the race condition described in the PR objectives.
158-162: Good: Focus handler now uses the shared lock.Wrapping the
refreshSessionTokencall inthis.tokenRefreshLock.acquireLockAndRun()prevents concurrent token fetches when multiple tabs gain focus simultaneously or when focus events fire while the poller is already refreshing.The updated comment clearly explains the cross-tab coordination mechanism.
47-47: Properties are properly initialized in the constructor — no issue exists.The properties
clientUatandsessionCookieare declared without explicit initialization on lines 47 and 50, but they are both assigned values in the constructor (lines ~88–95 viacreateClientUatCookie()andcreateSessionCookie()). TypeScript'sstrictPropertyInitializationrule only flags properties that are declared without initialization AND never assigned in the constructor. Since these properties are assigned before any use, they comply with strict mode requirements and require no changes.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/clerk-js/src/core/auth/safeLock.ts (2)
42-46: Memory leak: event listener is never removed.Each call to
SafeLock()adds a newbeforeunloadlistener that is never cleaned up. IfSafeLockis called multiple times (e.g., during hot reloads in development), listeners accumulate.Consider returning a cleanup function or ensuring
SafeLockis only instantiated once per key:export function SafeLock(key: string): SafeLockReturn { const lock = new Lock(); + const handleBeforeUnload = async () => { + await lock.releaseLock(key); + }; + // Release any held locks when the tab is closing to prevent deadlocks // eslint-disable-next-line @typescript-eslint/no-misused-promises - window.addEventListener('beforeunload', async () => { - await lock.releaseLock(key); - }); + window.addEventListener('beforeunload', handleBeforeUnload);Since this is scoped to single-instance usage (via
AuthCookieService), this is low-risk but worth noting for future reuse.
48-74: Consider adding explicit return type annotation for type safety.Per coding guidelines, explicit return types should be defined for functions. While TypeScript can infer the return type, an explicit annotation would improve clarity and catch type mismatches earlier.
- const acquireLockAndRun = async (cb: () => Promise<unknown>) => { + const acquireLockAndRun = async (cb: () => Promise<unknown>): Promise<unknown> => {packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (2)
82-96: Consider testing lock acquisition failure scenario.The tests cover the happy path well, but there's no test for when
acquireLockAndRunreturnsfalse(timeout/failure). This edge case is part of theSafeLockReturncontract.it('handles lock acquisition timeout gracefully', async () => { const sharedLock: SafeLockReturn = { acquireLockAndRun: vi.fn().mockResolvedValue(false), // Lock timeout }; const poller = new SessionCookiePoller(sharedLock); const callback = vi.fn().mockResolvedValue(undefined); poller.startPollingForSessionToken(callback); // Callback should not be invoked when lock returns false expect(callback).not.toHaveBeenCalled(); poller.stopPollingForSessionToken(); });
100-121: Unnecessaryasyncon test function.The test doesn't use
awaitat the top level. While not harmful, removingasyncwould be cleaner.- it('allows restart after stop', async () => { + it('allows restart after stop', () => {packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
83-104: Test name is misleading - mock doesn't actually simulate sequential execution.The test "mock lock can simulate sequential execution" fires both promises concurrently via
Promise.all. The mock implementation executes callbacks in parallel, not sequentially as a real lock would. This doesn't validate actual sequential behavior.Consider either:
- Renaming to clarify it tests concurrent invocation tracking
- Or implementing actual sequential simulation with a queue
- it('mock lock can simulate sequential execution', async () => { + it('tracks multiple concurrent lock invocations', async () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/clerk-js/src/core/auth/AuthCookieService.ts(6 hunks)packages/clerk-js/src/core/auth/SessionCookiePoller.ts(1 hunks)packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts(1 hunks)packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/auth/SessionCookiePoller.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (2)
packages/clerk-js/src/core/auth/safeLock.ts (1)
SafeLockReturn(6-15)packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
SessionCookiePoller(27-60)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLock(39-77)SafeLockReturn(6-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (34)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (machine, chrome, RQ)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Static analysis
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (11)
packages/clerk-js/src/core/auth/safeLock.ts (2)
3-15: JSDoc documentation added - addresses past review feedback.The interface now has proper JSDoc documentation explaining the cross-tab lock coordination purpose and the
acquireLockAndRunmethod behavior including the timeout return value.
17-39: JSDoc documentation added - addresses past review feedback.The function now has comprehensive JSDoc including purpose, parameters, return type, and a practical example demonstrating cross-tab coordination.
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (2)
6-14: LGTM - Clean test setup.Proper use of
beforeEach/afterEachfor fake timer management and mock restoration.
16-64: Good coverage for shared lock coordination pattern.These tests effectively validate the core fix - that an external lock can be injected and shared between the poller and focus handler, which is the key mechanism for preventing the race condition.
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (2)
6-23: LGTM - Good interface contract tests.These tests validate the API surface and mockability of
SafeLockReturn, which is important for the dependency injection pattern used inSessionCookiePoller.
25-45: Good defensive testing with environment check.The conditional skip for environments without Web Locks API is appropriate. The test validates the callback is invoked and the result propagates correctly.
packages/clerk-js/src/core/auth/AuthCookieService.ts (5)
23-25: LGTM - Clean imports for the new lock coordination feature.Proper separation of type import (
SafeLockReturn) from value imports (SafeLock,REFRESH_SESSION_TOKEN_LOCK_KEY).
51-54: Good documentation for the shared lock member.The JSDoc clearly explains the purpose of
tokenRefreshLockfor cross-tab coordination.
75-78: Lock creation is appropriately positioned.Creating the lock early in the constructor ensures it's available before
refreshTokenOnFocus()andstartPollingForToken()are called.
138-142: Correctly passes shared lock to poller.The poller now receives the same
tokenRefreshLockthat the focus handler uses, enabling cross-tab coordination.
152-168: Core fix: Focus handler now uses shared lock - LGTM.This is the key change that fixes the race condition. By wrapping
refreshSessionTokenintokenRefreshLock.acquireLockAndRun(), the focus handler will coordinate with both the poller and other tabs, preventing duplicate API calls.The comment clearly explains the coordination mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/clerk-js/src/core/resources/Session.ts (1)
45-64: Potential memory leak:tokenLocksMap grows unboundedly.The
tokenLocksMap accumulates entries for each unique tokenId (session + org + template combination) but never removes them. Over a long-lived session where users switch organizations or use different JWT templates, this could lead to memory growth.Consider adding a cleanup mechanism, such as:
- Using a
WeakMapif the key could be an object- Adding a maximum size with LRU eviction
- Clearing entries when the session ends
For now, this is low risk since each entry is lightweight (just a SafeLock instance), but worth noting for long-term maintenance.
packages/clerk-js/src/core/auth/safeLock.ts (1)
18-22: Multiplebeforeunloadlisteners accumulate with each SafeLock instance.Since
SafeLockis called once per unique tokenId (viagetTokenLockin Session.ts), a newbeforeunloadlistener is registered for each lock instance. Over time with many different token types, this accumulates listeners.Consider:
- Moving the listener registration outside the function (module-level, once)
- Tracking all lock instances in a module-level registry and releasing them in a single listener
- Using
{ once: true }isn't suitable here since we need it to fire every time+const lockRegistry = new Map<string, Lock>(); + +// Single listener to release all locks on tab close +window.addEventListener('beforeunload', async () => { + for (const [key, lock] of lockRegistry) { + await lock.releaseLock(key); + } +}); + export function SafeLock(key: string) { const lock = new Lock(); - - // Release any held locks when the tab is closing to prevent deadlocks - // eslint-disable-next-line @typescript-eslint/no-misused-promises - window.addEventListener('beforeunload', async () => { - await lock.releaseLock(key); - }); + lockRegistry.set(key, lock);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/fix-token-refresh-race-condition.md(1 hunks)packages/clerk-js/src/core/auth/SessionCookiePoller.ts(2 hunks)packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(1 hunks)packages/clerk-js/src/core/resources/Session.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/auth/SessionCookiePoller.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (1)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
SessionCookiePoller(11-39)
packages/clerk-js/src/core/auth/safeLock.ts (1)
packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
.changeset/fix-token-refresh-race-condition.md (1)
1-14: LGTM!The changeset provides clear and comprehensive documentation of the race condition fix, including the key implementation details: cross-tab locking, per-tokenId coordination, double-checked locking pattern, and graceful timeout handling.
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (1)
1-142: LGTM!Comprehensive test coverage for
SessionCookiePollerwith well-structured tests covering:
- Immediate callback execution on start
- Prevention of concurrent polling sessions
- Proper stop and restart behavior
- Correct scheduling that waits for callback completion
The use of fake timers and proper cleanup in each test is appropriate.
packages/clerk-js/src/core/resources/Session.ts (1)
416-473: Well-implemented double-checked locking pattern.The implementation correctly:
- Checks cache before acquiring lock (fast path)
- Re-checks cache after acquiring lock (prevents duplicate fetches when another tab populated it)
- Caches the promise immediately to prevent concurrent in-tab duplicates
- Holds the lock until the token is resolved and cached
packages/clerk-js/src/core/auth/safeLock.ts (1)
29-59: Solid lock acquisition implementation with appropriate degraded mode handling.The implementation correctly:
- Uses Web Locks API in secure contexts with abort-based timeout
- Falls back to localStorage-based locking in non-secure contexts
- Ensures operations complete in degraded mode rather than failing silently
- Properly releases locks in the fallback path via
finallyblockThe generic return type
<T>preserves type safety for callers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/clerk-js/src/core/resources/Session.ts (1)
38-64: Per-token SafeLock map looks good; consider lifecycle/typing polishThe per-tokenId SafeLock registry is a reasonable way to allow parallelism across token types while preventing duplicate fetches for the same token. One minor consideration:
tokenLocksis module-global and never pruned, so in very long-lived apps with many distinct tokenIds this Map could grow without bound. In practice tokenId cardinality is likely low, but if this ever becomes a concern, you could consider:
- Clearing entries when a session is ended/removed, or
- Introducing a small LRU or size cap.
You might also consider giving
getTokenLockan explicit return type (ReturnType<typeof SafeLock>) and extracting that into atype TokenLock = ReturnType<typeof SafeLock>;alias to make the intent clearer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/clerk-js/src/core/resources/Session.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/resources/Session.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/resources/Session.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/Session.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/resources/Session.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/resources/Session.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/resources/Session.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/resources/Session.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/clerk-js/src/core/resources/Session.ts (2)
394-407: Fast-path cache read before acquiring the lock is correctThe “check cache, then lock” flow looks solid: you avoid unnecessary lock acquisition on hot cache hits, preserve the existing TokenUpdate emission for cached tokens, and still normalize empty raw strings to
null. This aligns well with the new cross-tab locking behavior without adding contention to the common case.
409-465:skipCachesemantics are intentional and working correctly—no action neededVerification confirms the behavior matches the review's description and is intentional:
Behavior verified: When
skipCache=true, the code explicitly bypasses both pre-lock and post-lock cache reads (lines 395, 415:skipCache ? undefined : SessionTokenCache.get()), while still caching the result (line 450). ConcurrentskipCache=truecalls for the sametokenIdwill each trigger API requests rather than deduping in-flight promises.Intent is explicit: The conditional checks at lines 395 and 415 confirm this is deliberate—not an accidental oversight.
Usage context: Only one call site uses
skipCache=true(packages/clerk-js/src/core/clerk.ts:2774) during outage recovery. This scenario doesn't have multiple concurrent calls, so the edge case doesn't manifest in practice.The implementation is sound:
skipCachemeans "bypass my local cache read for this call" (ensuring fresh data), while still caching results for future calls (and forskipCache=falsecalls in other tabs). The double-check-lock pattern correctly prevents duplicate fetches betweenskipCache=falsecallers.
| // Cache the promise immediately to prevent concurrent calls from triggering duplicate requests | ||
| SessionTokenCache.set({ tokenId, tokenResolver }); |
There was a problem hiding this comment.
It's interesting that we have this here yet it doesn't solve the race condition. I suppose if N getToken calls are fired at the same time it would happen.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/fix-token-refresh-race-condition.md (1)
5-5: Refine the message to focus more on user impact than implementation.While this is already much improved, the mention of "cross-tab lock" remains implementation-focused. Consider rephrasing to emphasize the user-visible benefit (proper synchronization) rather than the mechanism (locking).
Suggested alternative:
-Fix race condition where multiple browser tabs could fetch session tokens simultaneously. `getToken()` now uses a cross-tab lock to coordinate token refresh operations +Fix race condition where multiple browser tabs could fetch session tokens simultaneously. Token refresh operations are now properly coordinated across tabs.This version emphasizes what users care about (proper synchronization) without describing the technical mechanism. Aligns with the feedback that release notes should be "light on implementation details and focus on the user impact."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.changeset/fix-token-refresh-race-condition.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/clerk-js/src/utils/__tests__/lru-map.test.ts (1)
5-73: Test coverage is comprehensive for core LRU behaviors.The tests cover the essential LRU functionality: basic operations, eviction, MRU promotion, updates, and edge case with maxSize=1.
Consider adding tests for these edge cases:
maxSizeof 0: The implementation'swhile (this.size >= this.maxSize)loop could behave unexpectedly or loop infinitely ifmaxSizeis 0.- Storing
undefinedas a value: Theget()implementation usesif (value !== undefined)to decide whether to refresh access, meaning entries withundefinedvalues won't be promoted to MRU position.+ it('handles storing undefined as a value', () => { + const map = new LruMap<string, number | undefined>(3); + map.set('a', undefined); + map.set('b', 2); + map.set('c', 3); + + // Access 'a' - should still exist but won't be promoted due to implementation + expect(map.get('a')).toBeUndefined(); + expect(map.has('a')).toBe(true); + }); + + it('throws or handles maxSize of 0', () => { + // Document expected behavior + const map = new LruMap<string, number>(0); + // This could cause issues - verify expected behavior + });packages/clerk-js/src/utils/lru-map.ts (3)
10-17: Edge case: entries withundefinedvalues won't receive MRU promotion.The condition
if (value !== undefined)prevents MRU promotion for entries that storeundefinedas a value. If a key maps toundefined,get()returns it but doesn't refresh its position, making it vulnerable to early eviction.For the current usage (storing
SafeLockobjects), this isn't an issue. However, for a generic utility class, consider usingsuper.has(key)instead:override get(key: K): V | undefined { - const value = super.get(key); - if (value !== undefined) { + if (super.has(key)) { + const value = super.get(key); super.delete(key); super.set(key, value); + return value; } - return value; + return undefined; }
5-8: Consider validatingmaxSizein the constructor.A
maxSizeof 0 or negative would result in an LruMap that can never store entries (all sets immediately evict). While the implementation handles this gracefully due to thebreakcondition, explicit validation would prevent misuse and clarify intent.constructor(private maxSize: number) { super(); + if (maxSize < 1) { + throw new Error('LruMap maxSize must be at least 1'); + } }
1-8: JSDoc could be enhanced with@paramand@exampletags.Per coding guidelines, public APIs should include comprehensive JSDoc with
@param,@returns,@throws, and@exampletags./** * A simple Map with LRU (Least Recently Used) eviction. * When the map exceeds maxSize, the oldest entries are removed. + * + * @template K - The type of map keys + * @template V - The type of map values + * @param maxSize - Maximum number of entries before eviction occurs (must be >= 1) + * + * @example + * const cache = new LruMap<string, number>(3); + * cache.set('a', 1); + * cache.get('a'); // Returns 1 and marks 'a' as most recently used */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/clerk-js/src/core/resources/Session.ts(3 hunks)packages/clerk-js/src/utils/__tests__/lru-map.test.ts(1 hunks)packages/clerk-js/src/utils/lru-map.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/utils/__tests__/lru-map.test.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/utils/__tests__/lru-map.test.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/utils/__tests__/lru-map.test.ts (1)
packages/clerk-js/src/utils/lru-map.ts (1)
LruMap(5-34)
packages/clerk-js/src/core/resources/Session.ts (9)
packages/clerk-js/src/utils/lru-map.ts (1)
LruMap(5-34)packages/clerk-js/src/core/auth/safeLock.ts (1)
SafeLock(15-62)packages/clerk-js/src/core/tokenCache.ts (1)
SessionTokenCache(413-413)packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)packages/clerk-js/src/core/events.ts (2)
eventBus(32-32)events(7-15)packages/clerk-js/rspack.config.js (1)
path(4-4)packages/clerk-js/src/core/resources/Client.ts (1)
path(161-163)packages/clerk-js/src/core/resources/User.ts (1)
path(111-113)packages/clerk-js/src/core/resources/Token.ts (1)
Token(7-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (34)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (machine, chrome, RQ)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Static analysis
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/clerk-js/src/core/resources/Session.ts (4)
46-64: Well-designed per-tokenId locking with bounded memory.The implementation correctly uses LRU eviction to prevent unbounded growth while maintaining per-tokenId locks for parallel fetches of different token types.
The design is safe even if a
SafeLockwrapper is evicted while the underlying browser lock is held—navigator.locks.requestis keyed by the string key (clerk.lock.getToken.${tokenId}), not the wrapper object. A newly createdSafeLockfor the same tokenId will still correctly coordinate with the existing browser lock.
394-407: LGTM: Fast path avoids lock overhead for cache hits.The unlocked cache check for immediate hits is the correct optimization pattern—it avoids cross-tab lock acquisition overhead for the common case where a valid token is already cached.
409-466: Double-checked locking pattern correctly implemented.The implementation follows the standard pattern:
- Check cache without lock (fast path)
- Acquire lock on miss
- Re-check cache after lock (another tab may have populated it)
- Fetch and cache if still missing
One observation: when
skipCacheistrue, the code still acquires the lock but always performs a fetch. This is intentional and correct—it ensures that even explicit cache-bypassing requests are serialized to prevent thundering herd on the API.
447-450: Based on my verification of the codebase, I can now provide the rewritten review comment:
The concern about rejected promises being cached is not valid — rejected promises are automatically cleaned up.
The
tokenResolverpromise is cached immediately at line 450, but ifToken.create()rejects, thetokenCache.tsimplementation automatically removes the entry via its.catch()handler (line 368:deleteKey()). This means failed token fetches do not persist in cache. On retry (line 130-137 of Session.ts), the cache miss triggers a freshToken.create()call, allowing retries to succeed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/clerk-js/src/core/auth/safeLock.ts (2)
30-32: Document theSafeLockReturnpublic interface with JSDoc
SafeLockReturnis exported and used as the public return type ofSafeLock, but it currently has no JSDoc. Project guidelines require JSDoc for public APIs, and a similar comment was raised earlier in this PR.Adding documentation here will clarify how callers should use the interface and what guarantees
acquireLockAndRunprovides (single execution, lock behavior, degraded mode semantics).For example:
+/** + * Public return type for {@link SafeLock}, representing a cross-tab lock handle. + * + * @public + */ export interface SafeLockReturn { - acquireLockAndRun: <T>(cb: () => Promise<T>) => Promise<T>; + /** + * Acquires a cross-tab lock and runs the provided callback while holding it. + * + * In degraded mode (e.g. lock acquisition timeout), the callback is still + * executed without the lock to avoid dropping the operation. + * + * @typeParam T - The resolved value type of the callback. + * @param cb - Async callback to run under the lock. + * @returns A promise that resolves or rejects with the callback’s result. + */ + acquireLockAndRun: <T>(cb: () => Promise<T>) => Promise<T>; }Please ensure the JSDoc tags and wording align with your existing documentation conventions (e.g. whether you prefer
@publicor another visibility tag).
34-42: ExtendSafeLockJSDoc with@returnsand an explicit usage exampleThe
SafeLockfunction’s JSDoc explains the purpose and@parambut is missing the@returnsand@exampletags required by the project’s API documentation guidelines and prior review feedback.To make the public API self-describing and consistent:
/** * Creates a cross-tab lock for coordinating exclusive operations across browser tabs. * * Uses Web Locks API in secure contexts (HTTPS), falling back to browser-tabs-lock * (localStorage-based) in non-secure contexts. * * @param key - Unique identifier for the lock (same key = same lock across all tabs) + * @returns A {@link SafeLockReturn} instance exposing {@link SafeLockReturn.acquireLockAndRun}. + * @example + * ```ts + * const lock = SafeLock('session-token-refresh'); + * await lock.acquireLockAndRun(async () => { + * // Perform token refresh or other critical section here. + * }); + * ``` */ export function SafeLock(key: string): SafeLockReturn {This keeps the implementation unchanged while improving discoverability for consumers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/Session.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/resources/__tests__/Session.test.ts (2)
packages/clerk-js/src/core/resources/Session.ts (1)
Session(66-467)packages/clerk-js/src/core/tokenCache.ts (1)
SessionTokenCache(413-413)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLock(42-87)SafeLockReturn(30-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/clerk-js/src/core/resources/__tests__/Session.test.ts (1)
430-492: New cache + skipCache tests accurately exercise token cache semanticsBoth tests correctly assert that:
- Once the token cache is populated, subsequent
getToken()calls reuse it without extra FAPI requests.skipCache: truebypasses the cache and forces an API call even whenSessionTokenCacheis hydrated.This is good, non-flaky coverage for the new locking/caching behavior.
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
60-118: SafeLock test suite nicely exercises interface, sharing, and lifecycle behaviorThe “shared lock pattern” and “beforeunload listener consolidation” tests are well structured:
- Demonstrate mocking
SafeLockReturnfor sequential execution and multi-consumer sharing.- Verify only a single
beforeunloadlistener is registered per module instance, and that a fresh import starts clean.This gives strong confidence in SafeLock’s intended usage patterns without over-coupling to implementation details.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/clerk-js/src/core/auth/safeLock.ts (2)
23-35: Add/restore full JSDoc forSafeLockReturnand completeSafeLockdocs
SafeLockReturn(Line 23) is exported, andSafeLock(Line 35) is a public helper, but:
SafeLockReturncurrently has no JSDoc at all.SafeLock’s JSDoc has@parambut is missing the required@returnsand@exampletags per the coding guidelines.Given these are part of the public locking API, please add/restore comprehensive JSDoc, e.g.:
+/** + * @public + * Return type for {@link SafeLock}, exposing a helper that serializes async work under a cross-tab lock. + * + * @example + * const { acquireLockAndRun } = SafeLock('token-refresh'); + * const result = await acquireLockAndRun(async () => { + * // refresh session token here + * return 'ok'; + * }); + */ export interface SafeLockReturn { + /** + * Acquires the underlying lock and runs the provided callback. + * + * @param cb - Async callback to execute while holding the lock. + * @returns A promise resolving to the callback result. + */ acquireLockAndRun: <T>(cb: () => Promise<T>) => Promise<T>; } /** * Creates a cross-tab lock for coordinating exclusive operations across browser tabs. * * Uses Web Locks API in secure contexts (HTTPS), falling back to browser-tabs-lock * (localStorage-based) in non-secure contexts. * * @param key - Unique identifier for the lock (same key = same lock across all tabs) + * @returns An object exposing {@link SafeLockReturn.acquireLockAndRun} to execute work under the lock. + * @example + * const safeLock = SafeLock('my-lock-key'); + * await safeLock.acquireLockAndRun(async () => { + * // critical section + * }); */ export function SafeLock(key: string): SafeLockReturn {This aligns with the project’s JSDoc requirements for public APIs.
41-54: Avoid double‑invoking the callback when Web Locks request rejectsIn the Web Locks path, the
catchblock at Line 51 treats any rejection fromnavigator.locks.requestas a lock timeout and callscb()again. Ifcb()itself throws or returns a rejected promise while holding the lock,navigator.locks.requestwill reject with that error, and this code will:
- Execute
cb()a second time (breaking non‑idempotent flows like token refresh).- Mis‑log the failure as a lock timeout/degraded mode.
- Potentially mask the original error with the second invocation’s outcome.
You should distinguish lock‑acquisition timeouts (AbortError) from callback errors and only fall back to degraded mode for the former; all other errors should be re‑thrown. Also, the timeout should be cleared in both success and error paths.
A possible fix:
- try { - return await navigator.locks.request(key, { signal: controller.signal }, async () => { - clearTimeout(lockTimeout); - return await cb(); - }); - } catch { - debugLogger.warn('Lock acquisition timed out, proceeding without lock (degraded mode)', { key }, 'safeLock'); - return await cb(); - } + try { + return await navigator.locks.request(key, { signal: controller.signal }, async () => { + clearTimeout(lockTimeout); + return await cb(); + }); + } catch (error) { + clearTimeout(lockTimeout); + + // Only fall back to degraded mode when the lock acquisition itself was aborted/timed out. + if (error instanceof DOMException && error.name === 'AbortError') { + debugLogger.warn( + 'Lock acquisition timed out, proceeding without lock (degraded mode)', + { key }, + 'safeLock', + ); + return await cb(); + } + + // Propagate callback errors and unexpected failures. + throw error; + }This preserves the degraded‑mode behavior for genuine lock timeouts while ensuring callback errors are propagated and not retried. You may want to add/adjust tests in
safeLock.test.tsto cover: (1) a callback that rejects under Web Locks is invoked exactly once and the error bubbles, and (2) an AbortError timeout falls back to degraded mode and runscb()once.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/resources/__tests__/Session.test.ts (1)
430-482: New getToken cache/skipCache tests look good; consider a stronger cross‑tab simulationThe two added tests correctly verify:
- Cache reuse so subsequent
getToken()calls avoid extra API requests.- That
skipCache: truebypasses the cache and still returns the expected token.One nit: the
"uses cache populated by another source during lock wait (simulates cross-tab)"test currently uses a singleSessioninstance with sequential calls, so it doesn’t actually exercise the per‑token lock behavior under concurrent callers (the main cross‑tab race the PR addresses).If you want to more directly cover the cross‑tab scenario, consider (optionally) updating that test to:
- Use two
Sessioninstances (or one per “tab”) sharingSessionTokenCache.- Call
Promise.all([session1.getToken(), session2.getToken()])and assert that only one API request occurs and both callers receive the same token.As‑is, the tests are valid and safe to keep.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts(2 hunks)packages/clerk-js/src/core/auth/safeLock.ts(1 hunks)packages/clerk-js/src/core/resources/Session.ts(2 hunks)packages/clerk-js/src/core/resources/__tests__/Session.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/clerk-js/src/core/auth/SessionCookiePoller.ts
- packages/clerk-js/src/core/resources/Session.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/auth/safeLock.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/resources/__tests__/Session.test.ts (3)
packages/clerk-js/src/test/core-fixtures.ts (1)
clerkMock(249-256)packages/clerk-js/src/core/resources/Session.ts (1)
Session(57-448)packages/clerk-js/src/core/tokenCache.ts (1)
SessionTokenCache(413-413)
packages/clerk-js/src/core/auth/safeLock.ts (1)
packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (machine, chrome, RQ)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Static analysis
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/auth/safeLock.ts (2)
packages/shared/src/telemetry/collector.ts (1)
window(279-317)packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLock(35-74)SafeLockReturn(23-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
packages/clerk-js/src/core/auth/safeLock.ts (3)
51-58: Error handling correctly distinguishes AbortError from callback errors.The fix properly addresses the previous concern about double-invocation. Now callback errors are re-thrown while only
AbortError(timeout) triggers degraded mode. This ensures non-idempotent operations aren't executed twice.
23-25: JSDoc documentation addressed.The interface and function now have appropriate JSDoc documentation as requested in previous reviews.
36-39: The original concern about orphaning locks is incorrect and not applicable.SafeLock results are always cached in Session.ts (the only production caller), so the same key will never call SafeLock multiple times. Additionally, even if the same key were reused, it would not cause orphaned locks because:
- The
Lockclass frombrowser-tabs-lockuses the stringkeyparameter to manage lock state, not the Lock instance itself- Line 61 demonstrates that a single Lock instance can acquire/release multiple different keys via
lock.acquireLock(key)andlock.releaseLock(key)- The beforeunload cleanup iterates all entries in the
activeLocksmap and callslock.releaseLock(key)for each, releasing the associated key regardless of which Lock instance stored itNo change needed to the implementation.
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (3)
38-57: Web Locks test skip condition now correctly includesisSecureContext.The skip condition at line 41 now checks
!isSecureContext, aligning with the implementation logic. This addresses the previous review comment.
176-205: Thorough testing of listener consolidation and module state.The tests properly verify that only one
beforeunloadlistener is registered regardless of how many locks are created, and that fresh module imports start with clean state.
60-117: Good coverage of shared lock patterns with mocks.The mock-based tests effectively demonstrate the intended usage pattern where multiple components share a lock via the
SafeLockReturninterface, validating sequential execution and correct invocation counts.
|
@brkalow mind taking another look? |
| if (shouldDispatchTokenUpdate) { | ||
| eventBus.emit(events.TokenUpdate, { token: cachedToken }); |
There was a problem hiding this comment.
❓ what's the purpose of this extra token update emit?
jacekradko
force-pushed
the
fix/token-poller-race-condition
branch
from
6d15034 to
3303971
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/clerk-js/src/core/auth/safeLock.ts (2)
65-73: Inconsistent error handling between lock paths.The browser-tabs-lock fallback (lines 65-71) propagates callback errors naturally via the try/finally pattern, while the Web Locks path (lines 53-62) swallows all errors. This creates inconsistent behavior depending on which lock mechanism is used.
Consider aligning the error handling so both paths propagate callback errors consistently.
48-62: Callback errors are silently swallowed and mixed with lock acquisition failures.The catch block at lines 58–61 indiscriminately catches all rejections—both lock acquisition failures (DOMException with AbortError, SecurityError, etc.) and errors thrown by the callback itself. When
cb()throws, the error is swallowed andfalseis returned instead of propagating the error to the caller.This masks bugs in callbacks, makes debugging difficult, and prevents callers from distinguishing between "lock not acquired" and "lock acquired but callback failed." The JSDoc at line 12 also documents only timeout behavior, not the full contract.
Additionally,
clearTimeout(lockTimeout)is only called on success (line 55), not in the error path, potentially leaking timeouts if lock acquisition fails quickly.Distinguish lock failures from callback errors:
return await navigator.locks .request(key, { signal: controller.signal }, async () => { clearTimeout(lockTimeout); return await cb(); }) - .catch(() => { + .catch((error) => { + clearTimeout(lockTimeout); + + // Only return false for lock acquisition failures (timeout/abort) + if (error instanceof DOMException && error.name === 'AbortError') { - // Lock request was aborted (timeout) or failed - // Return false to indicate lock was not acquired (matches browser-tabs-lock behavior) - return false; + return false; + } + + // Propagate callback errors + throw error; });
♻️ Duplicate comments (1)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
26-30: Align skip condition with implementation'sisSecureContextcheck.The implementation at
safeLock.ts:49checks'locks' in navigator && isSecureContextbefore using Web Locks. This test only checks fornavigator.locks:if (!('locks' in navigator) || !navigator.locks) { return; }If
navigator.locksexists butisSecureContextis false, the implementation uses the fallback path, but this test will still run and may exhibit unexpected behavior.Update the skip condition to include
isSecureContext:-if (!('locks' in navigator) || !navigator.locks) { +if (!('locks' in navigator) || !navigator.locks || !isSecureContext) { return; }
🧹 Nitpick comments (1)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
83-104: Consider clarifying test intent.This test uses a mock that executes callbacks immediately without actual lock serialization. While it validates the interface and call counts, it doesn't demonstrate true cross-tab lock behavior where the second call would wait for the first.
The test title "mock lock can simulate sequential execution" is accurate - it shows how to create mocks for component tests. Consider adding a brief comment to clarify this tests the mocking pattern, not actual lock semantics.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/fix-token-refresh-race-condition.md(1 hunks)packages/clerk-js/src/core/auth/AuthCookieService.ts(6 hunks)packages/clerk-js/src/core/auth/SessionCookiePoller.ts(1 hunks)packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts(1 hunks)packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/clerk-js/src/core/auth/tests/SessionCookiePoller.test.ts
- packages/clerk-js/src/core/auth/SessionCookiePoller.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/auth/AuthCookieService.ts (4)
packages/clerk-js/src/core/auth/cookies/clientUat.ts (1)
ClientUatCookieHandler(12-15)packages/clerk-js/src/core/auth/SessionCookiePoller.ts (2)
SessionCookiePoller(27-60)REFRESH_SESSION_TOKEN_LOCK_KEY(6-6)packages/clerk-js/src/core/auth/cookies/session.ts (2)
SessionCookieHandler(10-14)createSessionCookie(28-59)packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLockReturn(6-15)SafeLock(39-77)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLock(39-77)SafeLockReturn(6-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (10)
.changeset/fix-token-refresh-race-condition.md (1)
1-5: LGTM!The changeset is well-written with a clear, user-focused description of the fix. It appropriately explains the impact without diving into implementation details.
packages/clerk-js/src/core/auth/AuthCookieService.ts (5)
23-25: LGTM!The imports are well-organized, bringing in the necessary types and functions for cross-tab lock coordination.
75-78: LGTM!The shared lock is correctly initialized early in the constructor using the exported constant key. The comments clearly explain the coordination purpose.
138-143: LGTM!The poller correctly receives the shared lock, ensuring coordination with the focus handler.
152-168: LGTM!The focus handler correctly uses the shared lock to coordinate token refresh. The added comments clearly explain both the microtask timing rationale and the cross-tab coordination purpose.
92-98: LGTM!The initialization order changes are minor and don't affect functionality.
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
6-23: LGTM!The interface contract tests effectively validate the public API shape and demonstrate mockability for consumer tests.
packages/clerk-js/src/core/auth/safeLock.ts (3)
3-15: LGTM!The interface is well-documented with clear JSDoc explaining the cross-tab coordination behavior and return semantics.
17-39: LGTM!Excellent documentation with a practical example demonstrating the cross-tab coordination pattern.
42-46: LGTM!The beforeunload cleanup correctly prevents deadlocks when a tab holding a lock is closed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/clerk-js/src/core/auth/safeLock.ts (1)
39-44: Callback errors will trigger double execution in degraded mode.The catch block catches all exceptions from
navigator.locks.request, including errors thrown by the callbackcb(). Whencb()rejects while holding the lock, the Web Locks API propagates that rejection, which this catch block intercepts and then re-executescb()in "degraded mode" - resulting in the callback running twice.This is particularly problematic for token refresh operations which are not idempotent.
Apply this diff to distinguish lock acquisition failures from callback errors:
try { return await navigator.locks.request(key, { signal: controller.signal }, async () => { clearTimeout(lockTimeout); return await cb(); }); - } catch { - // Lock request was aborted (timeout) or failed - // Execute callback anyway in degraded mode to ensure operation completes - debugLogger.warn('Lock acquisition timed out, proceeding without lock (degraded mode)', { key }, 'safeLock'); - return await cb(); + } catch (error) { + clearTimeout(lockTimeout); + // Only fall back to degraded mode for lock acquisition failures (AbortError from timeout) + if (error instanceof DOMException && error.name === 'AbortError') { + debugLogger.warn('Lock acquisition timed out, proceeding without lock (degraded mode)', { key }, 'safeLock'); + return await cb(); + } + // Re-throw callback errors to prevent double execution + throw error; }
🧹 Nitpick comments (3)
packages/clerk-js/src/core/auth/safeLock.ts (1)
18-22:beforeunloadlistener accumulates on repeatedSafeLock()calls.Each invocation of
SafeLock()attaches a newbeforeunloadlistener. SincegetTokenLock()in Session.ts creates one lock per tokenId and caches it, this is somewhat mitigated. However, ifSafeLock()were called repeatedly with different keys or without caching, listeners would accumulate.Additionally, the async handler won't reliably complete before unload -
beforeunloaddoesn't await promises.Consider either:
- Documenting that callers must cache
SafeLockinstances (as done in Session.ts)- Using a module-level cleanup pattern with a single listener managing all locks
packages/clerk-js/src/core/resources/Session.ts (1)
50-50: Consider adding bounds to thetokenLocksmap.The map grows indefinitely as new tokenId combinations are encountered. While typical usage is bounded, applications with many organizations or JWT templates could accumulate entries over extended sessions.
Consider adding an LRU-style eviction or clearing stale locks when they haven't been used recently:
const MAX_TOKEN_LOCKS = 50; function getTokenLock(tokenId: string) { let lock = tokenLocks.get(tokenId); if (!lock) { // Simple eviction when limit reached if (tokenLocks.size >= MAX_TOKEN_LOCKS) { const firstKey = tokenLocks.keys().next().value; if (firstKey) tokenLocks.delete(firstKey); } lock = SafeLock(`clerk.lock.getToken.${tokenId}`); tokenLocks.set(tokenId, lock); } return lock; }packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (1)
111-115: Consider a cleaner pattern to avoid non-null assertion.The non-null assertion on
resolveCallback!works but could be avoided.Alternative pattern:
let resolveCallback: (() => void) | undefined; const callbackPromise = new Promise<void>(resolve => { resolveCallback = resolve; }); // Later: resolveCallback?.();Or use a deferred utility if available in the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/fix-token-refresh-race-condition.md(1 hunks)packages/clerk-js/src/core/auth/AuthCookieService.ts(2 hunks)packages/clerk-js/src/core/auth/SessionCookiePoller.ts(2 hunks)packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(1 hunks)packages/clerk-js/src/core/resources/Session.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/fix-token-refresh-race-condition.md
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/SessionCookiePoller.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/SessionCookiePoller.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/SessionCookiePoller.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/SessionCookiePoller.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/SessionCookiePoller.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/SessionCookiePoller.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/SessionCookiePoller.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts
🧬 Code graph analysis (4)
packages/clerk-js/src/core/resources/Session.ts (7)
packages/clerk-js/src/core/auth/safeLock.ts (1)
SafeLock(15-62)packages/clerk-js/src/core/tokenCache.ts (1)
SessionTokenCache(413-413)packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)packages/clerk-js/src/core/events.ts (2)
eventBus(32-32)events(7-15)packages/clerk-js/src/core/resources/Client.ts (1)
path(161-163)packages/clerk-js/src/core/resources/User.ts (1)
path(111-113)packages/clerk-js/src/core/resources/Token.ts (1)
Token(7-57)
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (1)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
SessionCookiePoller(11-39)
packages/clerk-js/src/core/auth/AuthCookieService.ts (3)
packages/clerk-js/src/core/auth/cookies/clientUat.ts (2)
ClientUatCookieHandler(12-15)createClientUatCookie(23-65)packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
SessionCookiePoller(11-39)packages/clerk-js/src/core/auth/cookies/session.ts (2)
SessionCookieHandler(10-14)createSessionCookie(28-59)
packages/clerk-js/src/core/auth/safeLock.ts (1)
packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (machine, chrome, RQ)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Static analysis
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
packages/clerk-js/src/core/resources/Session.ts (2)
45-64: LGTM - Well-implemented double-checked locking pattern.The per-tokenId lock approach correctly allows different token types to be fetched in parallel while preventing duplicate fetches for the same token. The JSDoc comments clearly explain the design rationale.
441-444: Good pattern: Caching the promise prevents concurrent duplicate requests.Caching
tokenResolverbefore awaiting ensures that concurrent calls within the same tab will share the same in-flight request rather than triggering multiple API calls.packages/clerk-js/src/core/auth/AuthCookieService.ts (1)
81-87: Initialization reordering looks correct.The reordering of cookie handler creation (
clientUat→devBrowser→sessionCookie) doesn't introduce any dependency issues since these handlers are independent. The property order in thecreateDevBrowsercall is a non-breaking change.packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (2)
1-14: Good test structure with proper setup and teardown.The test suite correctly uses fake timers and restores them after each test. This ensures test isolation and deterministic behavior.
108-140: Excellent test for callback completion sequencing.This test correctly validates that the next poll is only scheduled after the current callback completes, which is critical for preventing overlapping refresh operations.
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
5-10: Good documentation explaining the architectural decision.The JSDoc clearly explains that cross-tab coordination is handled in
Session.getToken(), making the poller's responsibility clear. This separation of concerns is a clean design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (2)
19-36: Add JSDoc documentation for public method.The method lacks JSDoc documentation explaining its purpose, parameters, and behavior. As per coding guidelines, public APIs should be documented with comprehensive JSDoc comments.
Apply this diff:
+ /** + * Starts polling for session token refresh at regular intervals. + * @param cb - Async callback to execute for each refresh attempt + */ public startPollingForSessionToken(cb: () => Promise<unknown>): void {
38-45: Add JSDoc documentation for public method.The method lacks JSDoc documentation. As per coding guidelines, public APIs should be documented with comprehensive JSDoc comments.
Apply this diff:
+ /** + * Stops the session token polling. + */ public stopPollingForSessionToken(): void {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/auth/SessionCookiePoller.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/auth/SessionCookiePoller.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/auth/SessionCookiePoller.ts
🧬 Code graph analysis (1)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (3)
7-12: Good architectural documentation.The JSDoc clearly explains that cross-tab coordination is now handled in
Session.getToken()rather than within the poller itself, which aligns with the PR's approach to fixing the race condition.
26-32: Error handling correctly implemented.The try/catch/finally block addresses the previous review concern about ensuring polling continues even if the callback fails. The finally block guarantees the timer is always rescheduled, and errors are properly logged for debugging.
38-45: Implementation is correct.The method properly clears the timer and resets the
initiatedflag, allowing the poller to be restarted. The!= nullcheck correctly handles the case wheretimerIdcould be 0.
Description
Fix race condition where multiple browser tabs could fetch session tokens simultaneously. The
refreshTokenOnFocushandler now uses the same cross-tab lock as the session token poller, preventing duplicate API calls when switching between tabs or when focus events fire while another tab is already refreshing the token.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.